Fix passing property and entity type names to ValueGenerated messages #29609
Conversation
Fixes #29517 Parameter-reordering works for exception messages, but doesn't work for log messages. Also checked there are no other cases of this.
There was a problem hiding this comment.
Hello @ajcvickers! I slept on putting up a PR for this issue as my first contribution to efcore, but I did some digging already and want to share what I found with the testing for these logging methods. I think that the log message tests are sufficient, but the EventData message tests appear incomplete. One thing I found for the EventData messages is CoreEventIdTest.Every_eventId_has_a_logger_method_and_logs_when_level_enabled(). This tests that the EventData text matches the logged messages. However, after looking into it further, it only accounts for the CoreResources.LogValueGenerated log EventDefinition and does not account for LogValueGeneratedSensitive, LogTempValueGenerated, and LogTempValueGeneratedSensitive. I observed the gap in testing by leaving out the fixes on src/EFCore/Diagnostics/CoreLoggerExtensions.cs lines 2864-65 and running all tests with no failures. This can be seen with other "Sensitive" log events because they all share a CoreEventId with the non-sensitive variants.
I wanted to ask your opinion on whether the gap is acceptable before putting up a PR. You beat me to the punch here though! I'm curious about your thoughts but understand if the issue is too small to warrant the discussion.
I'll look for another issue to be my first 😄
|
@tylerfelker I had already done a fix (not pushed) for this yesterday before you commented--sorry for stepping on your toes. Any improvements in the tests are always welcome. |
|
All good, @ajcvickers. There was no way to know without me commenting, so I'll be quicker next time. As far as the test improvements go, I can make a PR for it. I initially thought it would be nice if Expanding Any suggestions or preferences? Edit: I didn't mean to hijack your PR. It's not clear to me yet if the test improvements should be part of this original issue or broken out into a separate one. I can create a new issue if appropriate? |
|
@tylerfelker I don't have any specific preferences here. Keep in mind that we try to keep the tests in a state where they would work with localized messages, so, make sure not to hardcode any logger messages. |
Fixes #29517
Parameter-reordering works for exception messages, but doesn't work for log messages. Also checked there are no other cases of this.